Overload get() function for Optional type.
#9748
Conversation
yzh119
requested review from
areusch,
Hzfengsy,
junrushao,
kazum,
kparzysz-quic,
liangfu,
masahi,
tmoreau89,
tqchen,
vinx13 and
ZihengJiang
as code owners
|
Why close this PR? I'm very interested |
|
CI somehow failed, I'm trying to figure it out.
…On Wed, Dec 15, 2021, 1:49 AM Junru Shao ***@***.***> wrote:
Why close this PR? I'm very interested
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#9748 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZ2NM3O2FN62RIBIOPSPV3URBQELANCNFSM5KDBWGQA>
.
|
|
I think the CI was broken because of some random numerical issue and I'll re-trigger it, @junrushao1994 any further comments on this PR? |
|
Yes, it's a bit of flaky. Let me retrigger this |
|
@yzh119 Would you like to rebase and retrigger the CI? Thanks! |
|
@junrushao1994 done. |
| * \return The internal object pointer with container type of T. | ||
| * \note This function do not perform not-null checking. | ||
| */ | ||
| const ContainerType* get() const { return static_cast<ContainerType*>(data_.get()); } |
There was a problem hiding this comment.
Note that get is not a standard API as per stl style for Optional. While get is a standard stl style name. We might want to have a longer name in this case.
How about GetContainerPtr(use CamelCase to indicate that it is not STL compatible).
The comment can be updated to explain the behavior of nullopt case(return nullptr) and non-nullopt case(returns the container ptr)
There was a problem hiding this comment.
Thanks @tqchen for the valuable feedback! On the other hand, I would say this PR actually does is to simply improve the Optional::get method to return the underlying type instead of the plain Object pointer, and hence it might make sense to keep the name as it is. What do you think?
There was a problem hiding this comment.
Indeed, I didn't noticed that ObjectRef also have a get method. I think this sounds good to me then. Perhaps we can just add additional comments about the behavior
* upd * simplify * upd * fix * upd * fix docstring
* upd * simplify * upd * fix * upd * fix docstring
Currently
Optionaldon't have its ownget()implementation, so if we want to get the internal container pointer of an optional objectowith typeOptional<T>, we can either useo.value().get()(in the case it's defined) orstatic_cast<const T*>(o.get())(because the inheritedget()function has no type information). Neither of them are satisfactory if we want to get a pointer with typeconst T*.This PR overloads the
get()function ofOptional, which returnsnullptrwhen the object is not defined, and it's corresponding data pointer otherwise.cc @junrushao1994 @tqchen